fix(ledger): add internal foreign keys - #4652
Merged
Merged
Conversation
GAlexIHU
added
the
release-note/ignore
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds Ent edges and internal foreign keys for ledger breakage and customer-account relationships, introduces a ChangesLedger internal FK wiring
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Test as record_test.go
participant Helper as newExpiredRecord
participant Ledger as HistoricalLedger
participant Records as breakage.Record
Test->>Helper: newExpiredRecord(env, currency, name, features, expiresAt)
Helper->>Helper: GetSubAccountForRoute for FBO and breakage
Helper->>Ledger: CommitGroup(AnyTransactionInput at expiresAt)
Ledger-->>Helper: committed group and transaction IDs
Helper->>Records: build record with derived IDs and annotations
Records-->>Test: seeded expired record
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
GAlexIHU
marked this pull request as ready for review
July 6, 2026 14:41
Contributor
Greptile SummaryThis PR adds ledger-internal foreign keys and generated Ent relationships. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (2): Last reviewed commit: "docs(ledger): add FK migration guide" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openmeter/ent/schema/ledger_breakage_record.go`:
- Around line 130-187: The LedgerBreakageRecord schema is still missing indexes
on several foreign key fields, causing parent-table deletes to scan this table.
Update the Edges definition in LedgerBreakageRecord to ensure the remaining
FK-backed edges, especially source_transaction, breakage_transaction,
fbo_sub_account, breakage_sub_account, and release, are indexed in the generated
schema by adding the appropriate index annotations or schema indexes tied to
those field names.
In `@tools/migrate/migrations/20260706130134_add_ledger_internal_fks.up.sql`:
- Around line 4-6: The combined ALTER TABLE on ledger_customer_accounts is still
doing a blocking column type rewrite, so separate the account_id type change
from the foreign key addition. Update the migration around the
ledger_customer_accounts ALTER TABLE statement so the TYPE character(26) change
is its own step, then add the
ledger_customer_accounts_ledger_accounts_customer_accounts foreign key in a
separate NOT VALID statement.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 54c3c901-c48c-4c59-bb21-c21a8e2da563
⛔ Files ignored due to path filters (46)
openmeter/ent/db/client.gois excluded by!**/ent/db/**openmeter/ent/db/ledgeraccount.gois excluded by!**/ent/db/**openmeter/ent/db/ledgeraccount/ledgeraccount.gois excluded by!**/ent/db/**openmeter/ent/db/ledgeraccount/where.gois excluded by!**/ent/db/**openmeter/ent/db/ledgeraccount_create.gois excluded by!**/ent/db/**openmeter/ent/db/ledgeraccount_query.gois excluded by!**/ent/db/**openmeter/ent/db/ledgeraccount_update.gois excluded by!**/ent/db/**openmeter/ent/db/ledgerbreakagerecord.gois excluded by!**/ent/db/**openmeter/ent/db/ledgerbreakagerecord/ledgerbreakagerecord.gois excluded by!**/ent/db/**openmeter/ent/db/ledgerbreakagerecord/where.gois excluded by!**/ent/db/**openmeter/ent/db/ledgerbreakagerecord_create.gois excluded by!**/ent/db/**openmeter/ent/db/ledgerbreakagerecord_query.gois excluded by!**/ent/db/**openmeter/ent/db/ledgerbreakagerecord_update.gois excluded by!**/ent/db/**openmeter/ent/db/ledgercustomeraccount.gois excluded by!**/ent/db/**openmeter/ent/db/ledgercustomeraccount/ledgercustomeraccount.gois excluded by!**/ent/db/**openmeter/ent/db/ledgercustomeraccount/where.gois excluded by!**/ent/db/**openmeter/ent/db/ledgercustomeraccount_create.gois excluded by!**/ent/db/**openmeter/ent/db/ledgercustomeraccount_query.gois excluded by!**/ent/db/**openmeter/ent/db/ledgercustomeraccount_update.gois excluded by!**/ent/db/**openmeter/ent/db/ledgerentry.gois excluded by!**/ent/db/**openmeter/ent/db/ledgerentry/ledgerentry.gois excluded by!**/ent/db/**openmeter/ent/db/ledgerentry/where.gois excluded by!**/ent/db/**openmeter/ent/db/ledgerentry_create.gois excluded by!**/ent/db/**openmeter/ent/db/ledgerentry_query.gois excluded by!**/ent/db/**openmeter/ent/db/ledgerentry_update.gois excluded by!**/ent/db/**openmeter/ent/db/ledgersubaccount.gois excluded by!**/ent/db/**openmeter/ent/db/ledgersubaccount/ledgersubaccount.gois excluded by!**/ent/db/**openmeter/ent/db/ledgersubaccount/where.gois excluded by!**/ent/db/**openmeter/ent/db/ledgersubaccount_create.gois excluded by!**/ent/db/**openmeter/ent/db/ledgersubaccount_query.gois excluded by!**/ent/db/**openmeter/ent/db/ledgersubaccount_update.gois excluded by!**/ent/db/**openmeter/ent/db/ledgertransaction.gois excluded by!**/ent/db/**openmeter/ent/db/ledgertransaction/ledgertransaction.gois excluded by!**/ent/db/**openmeter/ent/db/ledgertransaction/where.gois excluded by!**/ent/db/**openmeter/ent/db/ledgertransaction_create.gois excluded by!**/ent/db/**openmeter/ent/db/ledgertransaction_query.gois excluded by!**/ent/db/**openmeter/ent/db/ledgertransaction_update.gois excluded by!**/ent/db/**openmeter/ent/db/ledgertransactiongroup.gois excluded by!**/ent/db/**openmeter/ent/db/ledgertransactiongroup/ledgertransactiongroup.gois excluded by!**/ent/db/**openmeter/ent/db/ledgertransactiongroup/where.gois excluded by!**/ent/db/**openmeter/ent/db/ledgertransactiongroup_create.gois excluded by!**/ent/db/**openmeter/ent/db/ledgertransactiongroup_query.gois excluded by!**/ent/db/**openmeter/ent/db/ledgertransactiongroup_update.gois excluded by!**/ent/db/**openmeter/ent/db/migrate/schema.gois excluded by!**/ent/db/**openmeter/ent/db/mutation.gois excluded by!**/ent/db/**tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (9)
openmeter/ent/schema/ledger_account.goopenmeter/ent/schema/ledger_breakage_record.goopenmeter/ent/schema/ledger_customer_account.goopenmeter/ent/schema/ledger_entry.goopenmeter/ent/schema/ledger_transaction.goopenmeter/ent/schema/ledger_transaction_group.goopenmeter/ledger/breakage/adapter/record_test.gotools/migrate/migrations/20260706130134_add_ledger_internal_fks.down.sqltools/migrate/migrations/20260706130134_add_ledger_internal_fks.up.sql
Comment on lines
+130
to
+187
| func (LedgerBreakageRecord) Edges() []ent.Edge { | ||
| return []ent.Edge{ | ||
| edge.From("source_transaction_group", LedgerTransactionGroup.Type). | ||
| Ref("source_breakage_records"). | ||
| Field("source_transaction_group_id"). | ||
| Immutable(). | ||
| Unique(), | ||
| edge.From("source_transaction", LedgerTransaction.Type). | ||
| Ref("source_breakage_records"). | ||
| Field("source_transaction_id"). | ||
| Immutable(). | ||
| Unique(), | ||
| edge.From("source_entry", LedgerEntry.Type). | ||
| Ref("source_breakage_records"). | ||
| Field("source_entry_id"). | ||
| Immutable(). | ||
| Unique(), | ||
| edge.From("breakage_transaction_group", LedgerTransactionGroup.Type). | ||
| Ref("breakage_records"). | ||
| Field("breakage_transaction_group_id"). | ||
| Required(). | ||
| Immutable(). | ||
| Unique(), | ||
| edge.From("breakage_transaction", LedgerTransaction.Type). | ||
| Ref("breakage_records"). | ||
| Field("breakage_transaction_id"). | ||
| Required(). | ||
| Immutable(). | ||
| Unique(), | ||
| edge.From("fbo_sub_account", LedgerSubAccount.Type). | ||
| Ref("fbo_breakage_records"). | ||
| Field("fbo_sub_account_id"). | ||
| Required(). | ||
| Immutable(). | ||
| Unique(), | ||
| edge.From("breakage_sub_account", LedgerSubAccount.Type). | ||
| Ref("breakage_records"). | ||
| Field("breakage_sub_account_id"). | ||
| Required(). | ||
| Immutable(). | ||
| Unique(), | ||
| edge.To("planned_releases", LedgerBreakageRecord.Type). | ||
| Annotations(entsql.OnDelete(entsql.Restrict)), | ||
| edge.To("release_reopens", LedgerBreakageRecord.Type). | ||
| Annotations(entsql.OnDelete(entsql.Restrict)), | ||
| edge.From("plan", LedgerBreakageRecord.Type). | ||
| Ref("planned_releases"). | ||
| Field("plan_id"). | ||
| Immutable(). | ||
| Unique(), | ||
| edge.From("release", LedgerBreakageRecord.Type). | ||
| Ref("release_reopens"). | ||
| Field("release_id"). | ||
| Immutable(). | ||
| Unique(), | ||
| } | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for index declarations covering the FK columns backing the new edges.
fd -e go . openmeter/ent/schema --exec rg -n 'index\.Fields\(' {} \;
echo "---- breakage record indexes ----"
rg -nP 'index\.Fields\([^)]*(source_transaction_id|breakage_transaction_id|fbo_sub_account_id|breakage_sub_account_id|release_id)' openmeter/ent/schema/ledger_breakage_record.go || echo "no matching FK-column indexes found in schema"Repository: openmeterio/openmeter
Length of output: 8239
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== schema outline =="
ast-grep outline openmeter/ent/schema/ledger_breakage_record.go --view expanded || true
echo
echo "== relevant schema sections =="
sed -n '1,260p' openmeter/ent/schema/ledger_breakage_record.go
echo
echo "== generated references for ledger_breakage_records =="
rg -n "ledger_breakage_records|source_transaction_id|breakage_transaction_id|fbo_sub_account_id|breakage_sub_account_id|release_id|plan_id|source_transaction_group_id|source_entry_id|breakage_transaction_group_id" openmeter/ent -g '!**/*.json' -g '!**/*.snap' || trueRepository: openmeterio/openmeter
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the generated schema for this entity if it exists.
fd -a 'ledger_breakage_record.go' openmeter/ent | sed -n '1,20p'
echo
for f in $(fd -a 'ledger_breakage_record.go' openmeter/ent); do
echo "== $f =="
rg -n "func \(.*\) Indexes\(\)|index\.Fields|Unique\(|source_transaction_id|breakage_transaction_id|fbo_sub_account_id|breakage_sub_account_id|release_id|plan_id|source_transaction_group_id|source_entry_id|breakage_transaction_group_id" "$f" || true
echo
doneRepository: openmeterio/openmeter
Length of output: 1605
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '3695,3810p' openmeter/ent/db/migrate/schema.goRepository: openmeterio/openmeter
Length of output: 4879
Add indexes for the remaining breakage FK columns. source_transaction_id, breakage_transaction_id, fbo_sub_account_id, breakage_sub_account_id, and release_id still don't get indexes in the generated ledger_breakage_records schema, so deletes on the parent tables can end up scanning this table.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openmeter/ent/schema/ledger_breakage_record.go` around lines 130 - 187, The
LedgerBreakageRecord schema is still missing indexes on several foreign key
fields, causing parent-table deletes to scan this table. Update the Edges
definition in LedgerBreakageRecord to ensure the remaining FK-backed edges,
especially source_transaction, breakage_transaction, fbo_sub_account,
breakage_sub_account, and release, are indexed in the generated schema by adding
the appropriate index annotations or schema indexes tied to those field names.
Source: Path instructions
Comment on lines
+4
to
+6
| -- modify "ledger_customer_accounts" table | ||
| -- Manual post-generation step: keep this foreign key NOT VALID for the same rollout reason as above. | ||
| ALTER TABLE "ledger_customer_accounts" ALTER COLUMN "account_id" TYPE character(26), ADD CONSTRAINT "ledger_customer_accounts_ledger_accounts_customer_accounts" FOREIGN KEY ("account_id") REFERENCES "ledger_accounts" ("id") ON UPDATE NO ACTION ON DELETE NO ACTION NOT VALID; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
Does PostgreSQL treat ALTER COLUMN TYPE from varchar to char(n) as a binary-coercible change that avoids a table rewrite?
💡 Result:
PostgreSQL does not treat an ALTER COLUMN TYPE from varchar to char(n) as a binary-coercible change, and it generally requires a table rewrite [1][2][3]. While PostgreSQL can avoid a full table rewrite when changing between certain types that are binary-coercible or are unconstrained domains over the new type (such as changing from varchar(n) to text or increasing the length of a varchar(n)), the conversion between varchar and char(n) is not considered binary-coercible [1][4]. The char(n) type requires that data be padded with spaces to the specified length n, whereas varchar stores only the string itself. Because the internal on-disk representation differs, PostgreSQL must rewrite the table to convert the values to the fixed-length, space-padded format required by char(n) [5][4]. This operation holds an ACCESS EXCLUSIVE lock on the table for the duration of the rewrite, which can cause significant downtime on large tables [4].
Citations:
- 1: https://www.postgresql.org/docs/19/sql-altertable.html
- 2: https://www.postgresql.org/docs/18/sql-altertable.html
- 3: https://www.postgresql.org/docs/16/sql-altertable.html
- 4: https://monpg.app/blog/postgresql-alter-column-type-no-rewrite
- 5: https://stackoverflow.com/questions/79860839/table-and-index-rewrites-when-changing-type-in-postgresql
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='tools/migrate/migrations/20260706130134_add_ledger_internal_fks.up.sql'
echo '--- file ---'
cat -n "$file"
echo
echo '--- surrounding migration files ---'
fd -a '20260706130134_add_ledger_internal_fks.*\.sql$|20260706130134_add_ledger_internal_fks\.up\.sql|20260706130134_add_ledger_internal_fks\.down\.sql' tools/migrate/migrations || trueRepository: openmeterio/openmeter
Length of output: 3004
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='tools/migrate/migrations/20260706130134_add_ledger_internal_fks.up.sql'
wc -l "$file"
sed -n '1,40p' "$file"Repository: openmeterio/openmeter
Length of output: 2803
Split the type change out from the FK add
ALTER COLUMN "account_id" TYPE character(26) still rewrites ledger_customer_accounts under an ACCESS EXCLUSIVE lock, so the NOT VALID FK doesn’t buy you the low-lock rollout benefit here. Split the type change into its own step.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/migrate/migrations/20260706130134_add_ledger_internal_fks.up.sql`
around lines 4 - 6, The combined ALTER TABLE on ledger_customer_accounts is
still doing a blocking column type rewrite, so separate the account_id type
change from the foreign key addition. Update the migration around the
ledger_customer_accounts ALTER TABLE statement so the TYPE character(26) change
is its own step, then add the
ledger_customer_accounts_ledger_accounts_customer_accounts foreign key in a
separate NOT VALID statement.
Source: Path instructions
GAlexIHU
enabled auto-merge (squash)
July 6, 2026 15:01
GAlexIHU
force-pushed
the
codex/ledger-data-layer-fks
branch
from
July 9, 2026 11:42
5d575c3 to
97a2dea
Compare
| @@ -0,0 +1,6 @@ | |||
| -- modify "ledger_breakage_records" table | |||
| -- Manual post-generation step: add these foreign keys as NOT VALID so the migration job enforces new writes without validating historical rows. Validate them separately after orphan preflight. | |||
| ALTER TABLE "ledger_breakage_records" ADD CONSTRAINT "ledger_breakage_records_ledger_breakage_records_planned_release" FOREIGN KEY ("plan_id") REFERENCES "ledger_breakage_records" ("id") ON UPDATE NO ACTION ON DELETE RESTRICT NOT VALID, ADD CONSTRAINT "ledger_breakage_records_ledger_breakage_records_release_reopens" FOREIGN KEY ("release_id") REFERENCES "ledger_breakage_records" ("id") ON UPDATE NO ACTION ON DELETE RESTRICT NOT VALID, ADD CONSTRAINT "ledger_breakage_records_ledger_entries_source_breakage_records" FOREIGN KEY ("source_entry_id") REFERENCES "ledger_entries" ("id") ON UPDATE NO ACTION ON DELETE RESTRICT NOT VALID, ADD CONSTRAINT "ledger_breakage_records_ledger_sub_accounts_breakage_records" FOREIGN KEY ("breakage_sub_account_id") REFERENCES "ledger_sub_accounts" ("id") ON UPDATE NO ACTION ON DELETE NO ACTION NOT VALID, ADD CONSTRAINT "ledger_breakage_records_ledger_sub_accounts_fbo_breakage_record" FOREIGN KEY ("fbo_sub_account_id") REFERENCES "ledger_sub_accounts" ("id") ON UPDATE NO ACTION ON DELETE NO ACTION NOT VALID, ADD CONSTRAINT "ledger_breakage_records_ledger_transaction_groups_breakage_reco" FOREIGN KEY ("breakage_transaction_group_id") REFERENCES "ledger_transaction_groups" ("id") ON UPDATE NO ACTION ON DELETE NO ACTION NOT VALID, ADD CONSTRAINT "ledger_breakage_records_ledger_transaction_groups_source_breaka" FOREIGN KEY ("source_transaction_group_id") REFERENCES "ledger_transaction_groups" ("id") ON UPDATE NO ACTION ON DELETE RESTRICT NOT VALID, ADD CONSTRAINT "ledger_breakage_records_ledger_transactions_breakage_records" FOREIGN KEY ("breakage_transaction_id") REFERENCES "ledger_transactions" ("id") ON UPDATE NO ACTION ON DELETE NO ACTION NOT VALID, ADD CONSTRAINT "ledger_breakage_records_ledger_transactions_source_breakage_rec" FOREIGN KEY ("source_transaction_id") REFERENCES "ledger_transactions" ("id") ON UPDATE NO ACTION ON DELETE RESTRICT NOT VALID; | |||
Contributor
There was a problem hiding this comment.
These NOT VALID constraints skip historical ledger_breakage_records, but PostgreSQL still checks them when one of those rows is later updated. Breakage records still have mutable columns such as deleted_at, annotations, amount, and updated_at, so an old row whose source_*, transaction, sub-account, or self-reference no longer exists can start failing routine soft-delete or maintenance updates after this migration. The validation guide is manual and does not enforce cleanup before the constraints are installed, so existing orphaned breakage projections can become rows that are readable but no longer updatable.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tools/migrate/migrations/20260709134422_add_ledger_internal_fks.up.sql
Line: 3
Comment:
**Breakage Orphans Freeze Updates**
These `NOT VALID` constraints skip historical `ledger_breakage_records`, but PostgreSQL still checks them when one of those rows is later updated. Breakage records still have mutable columns such as `deleted_at`, `annotations`, `amount`, and `updated_at`, so an old row whose `source_*`, transaction, sub-account, or self-reference no longer exists can start failing routine soft-delete or maintenance updates after this migration. The validation guide is manual and does not enforce cleanup before the constraints are installed, so existing orphaned breakage projections can become rows that are readable but no longer updatable.
How can I resolve this? If you propose a fix, please make it concise.
GAlexIHU
force-pushed
the
codex/ledger-data-layer-fks
branch
from
July 10, 2026 13:35
29ca255 to
f06b56c
Compare
turip
approved these changes
Jul 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ledger_customer_accounts.account_idandledger_breakage_recordsledger references.NOT VALIDfor deploy safety.Deploy Callout
After the migration has been applied, run the manual validation commands below after orphan preflight is clean:
Validation
nix develop --impure .#ci -c atlas migrate --env local hashnix develop --impure .#ci -c atlas migrate --env local diff ledger_internal_fks_checknix develop --impure .#ci -c make generatenix develop --impure .#ci -c go vet -tags=dynamic ./openmeter/ledger/...nix develop --impure .#ci -c env POSTGRES_HOST=127.0.0.1 go test -tags=dynamic ./openmeter/ledger/...Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Greptile Summary
This PR adds internal ledger foreign keys and rollout guidance. The main changes are:
Confidence Score: 5/5
This looks safe to merge from the follow-up changes reviewed here.
Important Files Changed
Reviews (5): Last reviewed commit: "fix(ledger): move FK migration to tail" | Re-trigger Greptile
Context used (3)